Skip to content

fix(react-swc): don't apply React Refresh wrapper for non JSX files#1226

Open
ArnaudBarre wants to merge 1 commit into
mainfrom
arnaud/swc-fix-refresh-wrapper
Open

fix(react-swc): don't apply React Refresh wrapper for non JSX files#1226
ArnaudBarre wants to merge 1 commit into
mainfrom
arnaud/swc-fix-refresh-wrapper

Conversation

@ArnaudBarre
Copy link
Copy Markdown
Member

Fixes #1225

Given that SWC already enable fast refresh only for jsx: true and that since this plugin was not running on .js, this feel a good and safe change. This could be a breaking change in this unlikely set of circumstances:

  • Using class component only
  • Using a file format other than JSX/TSX, with pre-processing that generates jsx_dev()
  • Configuring parserConfig to (id) => id.endsWith(".ext") ? ({ syntax: 'ecmascript', jsx: false }) : null

In that case hmr will go back to page reload. Changing to jsx: true will give back the previous behaviour

@ArnaudBarre
Copy link
Copy Markdown
Member Author

@sapphi-red I'm wondering if for the rolldown builtin refresh wrapper you access the ast and do something like:

let only_react_comp =
  !has_refresh &&
  // pseudo JS code
  ast.body.statements.some(
    (s) =>
      s.kind === 'ClassDeclaration' &&
      s.extends.name.match(/^(?:React\.)?(?:Pure)?Component$/),
  )

@sapphi-red
Copy link
Copy Markdown
Member

I guess we can also fix the error by changing this window with globalThis:

window.__registerBeforePerformReactRefresh = (cb) => {

@sapphi-red
Copy link
Copy Markdown
Member

@sapphi-red I'm wondering if for the rolldown builtin refresh wrapper you access the ast and do something like:

It doesn't have access to the AST here since the transform hook is string-in-string-out.

@ArnaudBarre
Copy link
Copy Markdown
Member Author

I guess we can also fix the error by changing this window with globalThis

For the past years I've found benefits to keep using window there because it allowed to find cases with bad configuration or people over bundling files in their workers. The idea was also that if one day we figure out HMR for workers, it will not be a breaking change currently the client can't run there.

But maybe the chance is so low that we get there that we should make it no-op to load the HMR client in a worker

@ArnaudBarre
Copy link
Copy Markdown
Member Author

It doesn't have access to the AST here since the transform hook is string-in-string-out
And I suppose it would cost to much memory to pass it around via some kind of context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@vitejs/plugin-react-swc Refresh wrapper erroneously added to non-component, error in Web Worker

2 participants